Codecov Report

❌ Patch coverage is 97.95918% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 93.23%. Comparing base (74e343b) to head (7d99406).

@@           Coverage Diff           @@
##             main    #5318   +/-   ##
=======================================
  Coverage   93.22%   93.23%           
=======================================
  Files         110      110           
  Lines       36599    36642   +43     
=======================================
+ Hits        34121    34162   +41     
- Misses       2478     2480    +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

I'm surprised this does not cause regressions, but in hindsight it should be ok

I agree. A possible issue is that abort from onRequestStart won't be called anymore if an AbortController is aborted after a request, but I'm not sure if it has any impact. We have decent test coverage for signals, GC, and cloning requests, and no WPTs have regressed either.

If this has no consequences, I think this should be upstreamed to the fetch spec. At the bare minimum, a note stating "the abort listener added can be removed here" could be added to the call sites.

At first, I didn’t think this would work, but after thinking it through more carefully, it actually seems to work quite well.

That said, since this behavior is outside the specification, we should proceed with caution. I found one edge case that is worth considering. In this scenario, determining when it is safe to detach the signal is very similar to how garbage collection works.

Specifically, the link can only be removed when:

• the fetch fails or is aborted; or
• all responses, including cloned responses, have been fully consumed, or have lost all references and been garbage-collected. This should be equivalent to the fetch controller associated with the fetch being fully disposed of.

As far as I can tell, only browsers handle this edge case correctly today; it has not yet been implemented in server-side fetch. This is not an immediate problem since it is currently unimplemented, but it will likely become an issue if we decide to implement it later.

const c = new AbortController();

const r = await fetch(
  new URL("/", globalThis.location?.href ?? "https://example.com/"),
  { signal: c.signal },
);

const r2 = r.clone(); // Clone it.

await r.arrayBuffer(); // Consume the entire body first to confirm the request has ended.

c.abort(); // Abort.

console.log((await r2.text()).slice(0, 100)); // This should throw an error because they are shared.

If this is a known limitation and we are comfortable with it for now, I’m fine with moving forward. However, it is something we will need to take into account if and when we implement this in the future.

If this is a known limitation and we are comfortable with it for now, I’m fine with moving forward. However, it is something we will need to take into account if and when we implement this in the future.

I can't find anything in the spec that says this case should throw. Browsers seem to be mishandling it.

I can't find anything in the spec that says this case should throw. Browsers seem to be mishandling it.

It seems like the browser is correct.
It makes sense if you think about it this way: the current implementation relies on a shared underlying fetch's body stream, which correctly aborts and works as long as the entire body isn't consumed first. This same logic can be applied to cases where the entire body is fully consumed.

const c = new AbortController();

const r = await fetch(
  new URL("/", globalThis.location?.href ?? "https://example.com/"),
  { signal: c.signal },
);

const r2 = r.clone(); // Clone it.

c.abort(); // Abort.

console.log((await r2.text()).slice(0, 100)); // This throw an error because they are shared.

It seems like the browser is correct.

Based on what?

Once the body is consumed, the stream is no longer readable and therefore is never canceled/errored, which propagates to the teed stream(s) (seemingly, I could be wrong as I'm not an expert in webstreams). In the 2nd example, since the first response's body's stream is still readable when the signal is aborted, it errors.

Spec:
#abort-fetch:

2. If request’s body is non-null and is readable, then cancel request’s body with error.
3. If response’s body is non-null and is readable, then error response’s body with error.

#concept-http-network-fetch:

18.2.1.2. If stream is readable, then...
18.2.2. Otherwise, if stream is readable,...

Good question. I agree that this is not Response.clone() inheriting or copying an AbortSignal; Response has no signal property.

The point is the body stream. Response.clone() clones the body by teeing its stream, so the cloned body is a branch derived from the same body stream, not an independent fetch. The Fetch Standard defines body cloning in terms of teeing the body stream, and Streams defines tee branches as observing errors from the original stream.

Your reading of abort fetch is reasonable if only the original r.body branch is considered the response body. After await r.text(), that branch is closed, so the “if body is readable” condition would not error it. But I think the subtle point is what counts as the relevant fetch-backed body after cloning.

Browsers appear to preserve the fetch-backed body relationship across the tee branches created by Response.clone(). So the unread cloned branch is not treated as a detached stream. Aborting the request can still error that branch, not because the Response inherited a signal, but because the cloned body remains part of the fetch-created body stream machinery.

So the browser behavior is better described as preserving the fetch-abort/error linkage across cloned body branches, not as Response-level signal inheritance.

Browsers are actually handling this correctly, but for different reasons - we aren't teeing the streams 'properly', and I'm not sure we are able to without access to ReadableStreamTee.

#rs-tee (webstreams) (this is how ReadableStream.prototype.tee is implemented):

Return ? ReadableStreamTee(this, false).

#concept-body-clone (fetch):

Let « out1, out2 » be the result of teeing body’s stream.

"teeing" links to #readablestream-tee (webstreams) (how .clone() tees the streams):

return ? ReadableStreamTee(stream, true).

... which has the note:

Because we pass true as the second argument to ReadableStreamTee, the second branch returned will have its chunks cloned ... from those of the first branch. This prevents consumption of one of the branches from interfering with the other.

So there is a bug in undici, but this PR is not responsible for it, and will not cause any issues in implementing it if we ever have access to ReadableStreamTee from node core.

Thanks for clarifying — that makes sense. I agree this should be framed as a Fetch body-clone issue, not as Response.clone() inheriting an AbortSignal.

Fetch body cloning uses the Streams “teeing” operation corresponding to ReadableStreamTee(stream, true), while ReadableStream.prototype.tee() uses ReadableStreamTee(this, false). So plain stream.tee() cannot fully model Fetch’s body-clone semantics.

Given that, browsers are handling this correctly. This PR does not introduce the bug; it exposes an existing limitation that should be tracked separately until an equivalent of ReadableStreamTee(stream, true) is available.